Skip to content

Stop overwriting global_user display_name on admin REST calls (ARB-515 follow-up)#386

Merged
crthpl merged 3 commits intomainfrom
arb-515-fallback-display-name
Apr 13, 2026
Merged

Stop overwriting global_user display_name on admin REST calls (ARB-515 follow-up)#386
crthpl merged 3 commits intomainfrom
arb-515-fallback-display-name

Conversation

@crthpl
Copy link
Copy Markdown
Contributor

@crthpl crthpl commented Apr 13, 2026

Summary

Follow-up to #380. After merging the first ARB-515 fix, users like theo@reinsberg.info still showed their Kinde sub as their display name, even after interacting with cohorts. Three distinct bugs are at play here; this PR fixes them all and adds a backfill migration for already-broken rows.

  1. check_admin was overwriting display_name on every admin REST call. It called ensure_global_user(&claims.sub, &claims.sub, ...) unconditionally, so any admin hitting /api/admin/* (which is constant traffic in the admin UI) had their display_name silently reset to their Kinde sub. theo@reinsberg.info is a Kinde admin — that's why list_cohorts setting a good name was immediately clobbered. Switched to find_or_create_global_user, which still syncs is_kinde_admin but never touches the display name.

  2. WS auth used "Unknown" as a fallback and then called ensure_global_user. Same overwrite pattern: users without a name claim in their id_token would have their display_name replaced with "Unknown". The WS auth path now mirrors list_cohorts: trusted name → ensure_global_user; otherwise find_or_create_global_user with email (or sub) as a create-only placeholder. The per-cohort account creation below now reuses global_user.display_name directly.

  3. IdClaims required a name: String claim. Users whose Kinde account has no full name set were either failing token validation or ending up with an empty string. name is now Option<String>, plus new optional given_name and family_name fields and an IdClaims::resolve_name() helper that prefers name, then given_name + family_name.

Also:

  • list_cohorts now uses the resolved email as the create-time placeholder instead of the sub, so new users with no name claim still get a recognisable display name from the start.
  • New migration 005_backfill_placeholder_display_names.sql rewrites existing rows where display_name = kinde_id or display_name = 'Unknown' to the user's email when available, rescuing users already broken by the old behaviour without waiting for them to log in again.

Fixes ARB-515 (for real this time).

Test plan

  • cargo clippy --features dev-mode --all-targets clean
  • cargo test-all passes
  • Manual: verify theo@reinsberg.info shows a sensible display name after deploy + migration runs
  • Manual: log in as an admin, hit multiple admin endpoints, confirm display_name is not overwritten to the Kinde sub

Three follow-up bugs to the initial ARB-515 fix surfaced in production
(users like theo@reinsberg.info still showed their Kinde `sub` as their
display name even after my earlier PR and after being touched in cohort
flows):

1. `check_admin` in `main.rs` called `ensure_global_user(&sub, &sub, ...)`
   unconditionally. Since admins hit REST endpoints frequently and
   `ensure_global_user` updates `display_name` whenever it differs, every
   admin REST request silently overwrote the user's real display name back
   to their Kinde sub. Switch `check_admin` to `find_or_create_global_user`,
   which syncs `is_kinde_admin` but never touches the display name.

2. The WS auth path in `handle_socket.rs` used `"Unknown"` as a fallback
   when the id_token lacked a `name` claim and then called `ensure_global_user`
   with that fallback, overwriting good names with "Unknown" for any user
   whose Kinde account has no full-name claim. The WS auth flow now takes
   the same two-mode approach as `list_cohorts`: when there's a trusted
   id_token name we call `ensure_global_user`; otherwise we fall back to
   `find_or_create_global_user` with the email (or sub) as a create-time
   placeholder, which never clobbers an existing name. The cohort-db
   account creation below now reuses `global_user.display_name` directly.

3. `IdClaims` required a `name: String` claim, so users whose Kinde token
   omitted `name` entirely either failed validation or ended up with an
   empty string. Make it `Option<String>`, add `given_name`/`family_name`
   fields, and add `IdClaims::resolve_name()` which prefers `name`,
   falling back to a trimmed `given_name` + `family_name` concatenation.
   Both `validate_access_and_id` and `validate_id_token_for_sub` now go
   through `resolve_name()`.

Also: `list_cohorts` now uses the resolved email as the create-time
placeholder instead of the sub, so brand-new users without any id_token
name claim still land with a recognisable name from the start.

Finally, add a global-DB migration that backfills existing rows where
`display_name = kinde_id` or `display_name = 'Unknown'` with the user's
email when one is available, so users broken by the earlier behaviour
get rescued without having to wait for a login that re-populates them.
@crthpl crthpl requested a review from a team as a code owner April 13, 2026 22:09
@vercel
Copy link
Copy Markdown

vercel bot commented Apr 13, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
platform Ready Ready Preview, Comment Apr 13, 2026 10:38pm

Request Review

Following up on the previous ARB-515 commit: `ensure_global_user` still
overwrote `display_name` whenever the trimmed id_token name differed
from the stored value, which silently clobbered any name the user or an
admin had set through `/api/users/me/display-name` or
`/api/admin/users/:id/display-name` on the next login. From the user's
perspective, manual renames just "didn't stick".

- `ensure_global_user`: stop touching `display_name` on existing rows.
  The `name` parameter is now only used on creation. `email` and
  `is_kinde_admin` are still synced so email changes and Kinde admin
  revocations still propagate.

- `check_admin`: no longer calls `find_or_create_global_user`, which
  would insert a placeholder row with the Kinde sub as the display
  name for any admin who somehow hit an admin endpoint before the
  main app. It now uses a new `sync_is_kinde_admin_by_kinde_id`
  helper that looks the user up, syncs the Kinde admin flag in
  place, and returns `None` (→ 403) when the row doesn't exist.
  Admins always reach admin endpoints after `/api/cohorts`, which
  creates the row with a real name.

- `update_my_display_name`: no longer pre-creates the caller's row
  with their Kinde sub as the display name. It now looks the row up
  and returns 404 if missing, same reasoning as `check_admin`.

With these changes, the only remaining code paths that can write the
Kinde sub as a user's display_name are the creation fallbacks in
`list_cohorts` and the WS auth handler, and only when *both* the
id_token is missing/invalid *and* the access token carries no email —
a scenario that only arises for direct API callers or dev-mode test
tokens without an email field.
Neither the Kinde sub nor the user's email should ever surface as a
display name in the UI. Move placeholder generation into
`find_or_create_global_user` so callers can't accidentally reach for the
sub or email: the function now takes no `placeholder_name` argument and
generates an `Unnamed-XXXX` string via a new `generate_unnamed_placeholder`
helper (4 random alphanumeric chars, so two simultaneously-created
unnamed users in the same account list are still distinguishable).

Update `list_cohorts` and the WS auth handler to call the trimmed-down
signature, and rewrite the backfill migration to replace any existing
row where `display_name` equals the Kinde sub, the user's email, or
the literal `"Unknown"` with a fresh `Unnamed-XXXX` suffix per row
(`randomblob()` is volatile in SQLite, so it's re-evaluated per updated
row — verified manually against a throwaway DB).
@crthpl crthpl merged commit 95ff300 into main Apr 13, 2026
5 checks passed
briansmiley added a commit that referenced this pull request Apr 14, 2026
Reverts commits 7cd390c..c5f4008 (15 commits) to restore the
pre-multi-cohort state. Critical event tomorrow — rolling back to
last known stable state.

Reverted commits:
- 7cd390c Multi-cohort support with per-member initial balance (#358)
- 1a9259e Allow sudoed admins higher decimal precision (#370)
- 612f7e7 Add select all / clear buttons to transfer recipient multiselect (#350)
- ec96705 Link cohort members to existing users when added by email (#378)
- eed7a73 Consolidate admin page data loading into single /api/admin/overview (#381)
- eac92d0 Populate global user display name from id_token on login (ARB-515) (#380)
- c5461ee Remove redundant Refresh button from /admin page (#382)
- 77168e3 Sync Kinde admin role into global_user.is_admin (ARB-512) (#379)
- 685ee0c Remove dead get_market_positions and fix 0.5.0 ImportError (#385)
- e9d49f7 Constrain initial balance inputs to numeric values (#383)
- 95ff300 Stop overwriting global_user display_name on admin REST calls (#386)
- 0163100 Show user email on admin page while editing display name (#387)
- 85f2c5f Fix user selection dropdown hover when multiple users have same name (#388)
- 3fedb38 Add admins to cohort member list on access (ARB-510) (#389)
- c5f4008 Fan out display-name rename to every cohort (ARB-513) (#390)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
briansmiley added a commit that referenced this pull request Apr 14, 2026
Reverts commits 7cd390c..c5f4008 (15 commits) to restore the
pre-multi-cohort state. Critical event tomorrow — rolling back to
last known stable state.

Reverted commits:
- 7cd390c Multi-cohort support with per-member initial balance (#358)
- 1a9259e Allow sudoed admins higher decimal precision (#370)
- 612f7e7 Add select all / clear buttons to transfer recipient multiselect (#350)
- ec96705 Link cohort members to existing users when added by email (#378)
- eed7a73 Consolidate admin page data loading into single /api/admin/overview (#381)
- eac92d0 Populate global user display name from id_token on login (ARB-515) (#380)
- c5461ee Remove redundant Refresh button from /admin page (#382)
- 77168e3 Sync Kinde admin role into global_user.is_admin (ARB-512) (#379)
- 685ee0c Remove dead get_market_positions and fix 0.5.0 ImportError (#385)
- e9d49f7 Constrain initial balance inputs to numeric values (#383)
- 95ff300 Stop overwriting global_user display_name on admin REST calls (#386)
- 0163100 Show user email on admin page while editing display name (#387)
- 85f2c5f Fix user selection dropdown hover when multiple users have same name (#388)
- 3fedb38 Add admins to cohort member list on access (ARB-510) (#389)
- c5f4008 Fan out display-name rename to every cohort (ARB-513) (#390)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant